Skip to content

Feat/set data by expression suggestions#1801

Open
ivarne wants to merge 8 commits into
feature/set-data-field-by-expressionfrom
feat/set-data-by-expression-suggestions
Open

Feat/set data by expression suggestions#1801
ivarne wants to merge 8 commits into
feature/set-data-field-by-expressionfrom
feat/set-data-by-expression-suggestions

Conversation

@ivarne

@ivarne ivarne commented Jun 8, 2026

Copy link
Copy Markdown
Member

Suggested changes to #1683

commit 9b1e757
Date: Mon Jun 8 23:21:16 2026 +0200

Unify GetResolvedKeys to always return all resolved keys

ExpressionValidation now checks for null by it self, not relying on isCalculating = false. (added a shared test for this)

commit be58d5d
Date: Mon Jun 8 23:07:20 2026 +0200

Get rid of LayoutEvaluatorStateInitializer in ExpressionValidator

commit b8f15ac
Date: Mon Jun 8 15:47:43 2026 +0200

Don't filter calcluations to only run when the display of the value is not hidden

commit 89840fa
Date: Mon Jun 8 15:42:09 2026 +0200

Fix merge conflict and don't use object[] for positional arguments

Copilot AI review requested due to automatic review settings June 8, 2026 21:28
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: af60e7f1-1aa1-4d08-83d6-f5657ac13e12

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/set-data-by-expression-suggestions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines expression-based validation and data-field calculation by simplifying evaluator state initialization, unifying resolved-key behavior, and ensuring expression validation skips null field values.

Changes:

  • Removed ILayoutEvaluatorStateInitializer usage from ExpressionValidator and DataModelFieldCalculator, relying on IInstanceDataAccessor.GetLayoutEvaluatorState() instead.
  • Unified resolved-key resolution by removing the isCalculating variants and making key resolution always include resolved keys (even when leaf values are null).
  • Updated expression evaluation API (EvaluateExpressionToExpressionValue) to accept IInstanceDataAccessor and ExpressionValue[] positional arguments; added a shared test case for “ignore when value is null”.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/Altinn.App.Core/Features/Validation/Default/ExpressionValidator.cs Removes state initializer dependency; adds explicit null short-circuit before evaluating validation expressions.
src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs Removes state initializer + hidden-field filtering; resolves keys via accessor state and evaluates expressions via new API.
src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs Changes EvaluateExpressionToExpressionValue to take IInstanceDataAccessor and ExpressionValue[].
src/Altinn.App.Core/Internal/Expressions/LayoutEvaluatorState.cs Removes GetResolvedKeys(..., isCalculating) overload.
src/Altinn.App.Core/Internal/Data/IFormDataWrapper.cs Removes GetResolvedKeys(..., isCalculating) extension and routes to unified resolution.
src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs Removes isCalculating behavior toggle and adjusts recursion to always resolve leaf keys.
test/Altinn.App.Core.Tests/Features/Validators/Default/ExpressionValidatorTests.cs Updates tests to no longer mock/init layout evaluator state initializer.
test/Altinn.App.Core.Tests/Features/DataProcessing/DataModelFieldCalculatorTests.cs Updates calculator tests for new constructor/signature and accessor setup.
test/Altinn.App.Core.Tests/Features/Validators/expression-validation-tests/shared/ignore-value-null.json Adds shared regression test fixture ensuring validations are ignored when value is null.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt Updates public API snapshot for removed/changed overloads and new evaluator signature.
test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_PdfFails_DataIsUnlocked.verified.txt Updates verified telemetry output after internal call graph changes.
test/Altinn.App.Api.Tests/Controllers/ProcessControllerTests.RunProcessNext_FailingValidator_ReturnsValidationErrors.verified.txt Updates verified telemetry output after internal call graph changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs
Comment thread src/Altinn.App.Core/Helpers/DataModel/DataModelWrapper.cs Outdated
Comment thread src/Altinn.App.Core/Features/DataProcessing/DataModelFieldCalculator.cs Outdated
Comment thread src/Altinn.App.Core/Internal/Expressions/ExpressionEvaluator.cs Outdated
@ivarne ivarne force-pushed the feat/set-data-by-expression-suggestions branch from 6144424 to d536d60 Compare June 10, 2026 09:49
@ivarne ivarne force-pushed the feat/set-data-by-expression-suggestions branch from d536d60 to 2054d48 Compare June 10, 2026 10:27
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants